🔍 Rust PR Review

Summary: Looks good — well-structured, security-conscious, and thoroughly tested. One version change worth confirming.

Findings

⚠️ Suggestions

• smoke-failure-reporter.lock.yml line 236 — AWF version downgrade: AWF_VERSION goes from 0.27.28 → 0.27.9 (a downgrade by patch number). The PR description mentions "replacement definitions" and a migration context, so this may be intentional. Just worth a double-check that 0.27.9 is the correct target for the new AgentPlayground service connection setup and not an accidental revert.

• mirror.ts:162 — diagnostic() redacts token but not URL form: The bearer token is redacted from diagnostic output via replaceAll(secret, "***") — good. However, if the ADO URL passed to push/ls-remote ever gets echoed in stderr (e.g., from a git error), the token won't be embedded there (it's in env, not argv) so this is fine as-is. Just noting the defense-in-depth is already correct.

✅ What Looks Good

• Token out of argv: Bearer auth is passed via GIT_CONFIG_VALUE_0 env var, not as a URL credential or command argument. The test explicitly asserts push?.args.join(" ")).not.toContain("secret-token"). Correct pattern.
• Fast-forward-only + post-push SHA verification: The push --porcelain without --force fails closed on non-FF, and the subsequent ls-remote verification confirms the remote HEAD matches — solid correctness guarantee.
• Shallow-checkout guard: Refusing to sync a shallow checkout prevents silent partial-history mirroring.
• cleanVar macro guard: Stripping unexpanded $(VAR) ADO macro tokens before using them is a good defensive practice in pipeline-executed scripts.
• Test coverage: The 7 test cases in mirror.test.ts cover all the important failure modes (non-main branch, shallow clone, non-FF rejection, SHA mismatch, unset victim repo).
• Rust test: test_smoke_failure_reporter_uses_registered_ado_names_and_staging_repo is a clean content-contract test that will catch accidental definition-name regressions.

Generated by Rust PR Reviewer for #1560 · 42.9 AIC · ⌖ 5.73 AIC · ⊞ 6.2K · ◷

Confirmed the AWF change is intentional: the current compiler constant is 0.27.9 (src/compile/common.rs), set by #1526 to restore host-access compatibility. Regenerating the reporter lock from v0.44.1 to v0.45.1 picked up that canonical version; no follow-up code change is needed.

Rebased onto current main at a59eda0 (#1563). git range-diff reports the cutover commit as unchanged (=), and the rebased targeted Rust/TypeScript tests plus trigger bundle build all pass.

🔍 Rust PR Review

Summary: Looks good — this is primarily a CI infrastructure migration (TypeScript, YAML, docs), with a small Rust integration test addition. No blocking issues found.

Findings

✅ What Looks Good

• mirror.ts — security design is solid. The ADO bearer token is passed exclusively via GIT_CONFIG_VALUE_0 (an in-process env var), never via argv or git config file. The diagnostic() helper scrubs the token from error output with .replaceAll(secret, "***"). The push is correctly --porcelain and fast-forward-only (no --force).

• mirror.ts — fail-closed semantics are correct. Shallow-checkout detection, non-main branch guard, and post-push SHA verification all return ok: false before victim scenarios run. The test suite covers each path.

• mirror.ts — timeout handling. SIGKILL after timeout, settled guard to avoid double-resolve, and the error event handler all look correct. clearTimeout is correctly called before resolving.

• runGit spawns with process.env spread. The base env is spread first, then the request env overrides it, so GIT_TERMINAL_PROMPT=0 / trace suppression take effect even if the host has them set to something else. Correct order.

• Rust test (test_smoke_failure_reporter_uses_registered_ado_names_and_staging_repo) asserts both positive presence and negative absence of colon-containing names — good double-sided guard.

⚠️ Suggestions

• mirror.ts:145 — unbounded stdout/stderr accumulation. stdout and stderr are accumulated as plain strings with no size cap. A misbehaving or hung git process could buffer large amounts of data before the timeout fires. Since MAX_DIAGNOSTIC_CHARS already truncates output for display, consider also capping the raw buffers (e.g., discard once > 4× MAX_DIAGNOSTIC_CHARS). Low severity given the controlled environment, but worth noting.

• mirror.ts:runMirrorSyncPreflight — redundant enabled-check. The function checks enabled(env.TRIGGER_E2E_SYNC_MIRROR) && !cleanVar(env.TRIGGER_E2E_VICTIM_REPO) and then immediately calls loadMirrorSyncConfig, which internally checks the same two conditions. The early-return path with the log message is only reachable when TRIGGER_E2E_SYNC_MIRROR=true but TRIGGER_E2E_VICTIM_REPO is empty — which is already handled silently by loadMirrorSyncConfig returning undefined. The explicit branch exists only to emit the log line, which is fine, but a comment explaining this intent would prevent a future reader from collapsing it.

• AWF downgrade (0.27.28 → 0.27.9) in smoke-failure-reporter.lock.yml. The PR description explains this follows fix(compile): downgrade AWF for host access #1526 restoring host-access compatibility, so this is intentional. Just confirm the lock file downgrade is tracked and will be bumped again once the host-access issue in 0.27.28 is resolved, so this doesn't become a forgotten floor.

Generated by Rust PR Reviewer for #1560 · 34.8 AIC · ⌖ 8.14 AIC · ⊞ 6.2K · ◷